are we short on bytes? the gh part is obvious to me as a reader, but I cannot figure out what cr means, and since this only appears to be documented inside this file, I’m not sure it would be clear from the package.json file, especially since it looks like a standard GitHub repo name.

perhaps something githubRepo would suffice? it doesn’t seem like the package.json value has anything specific to do with the Github Container Registry

I am new to this as well, so I found that GHCR stands for the GitHub Container Registry

I've added a note within the inlind docblock to explain this at the first occurrence of ghcrRepo. Does that address your concerns here @dmsnell?

I also changed the name of the package to gutenberg-wp-develop-build, making the full reference WordPress/gutenberg/gutenberg-wp-develop-build. It's more specific to the purpose, and hopefully won't lead to any confusion.

Within the GHCR, packages are published to a specific repository. While they are listed on a separate screen, you need to use the Org/Repository/package path to reference them.

at first I was going to ask about making this check before downloading, but I see it exists after downloading.

this is probably better because it informs someone that their files are out of date without eagerly downloading a new copy and replacing the old ones.

I think I also want to create a file in the root of the repository to store a hash of the entire gutenberg directory immediately after unzipping. Then the user can also be made aware of instances where the files in the gutenberg directory were modified, which may be unexpected.

this can backfire if we want to hash the entire directory. that can be an I/O-intensive task and either requires polling or reliable file watching.

with the solution already here: notify when expired, it gives agency to the user to download or ignore

Is this really streaming? It seems the stdout variable is populated with streaming, but the promise only resolves with the captured stdout when the child process is closed?

Updated, how does that look?

at this point it’s probably possible to remove all of the _Sync versions and stream the output directly into the file.

not essential, but it does feel like we’re possibly going out of our way to do more work to avoid a simpler default.

await fs.writeFile( zipPath, response.body );

I'm not deeply knowledgeable here, but Claude told me that this wouldn't work as you've written "since fs.promises.writeFile doesn't accept a ReadableStream — stream.pipeline is the correct API for this." Does this seem right now?

this can fail if there are uncommitted changes. what’s the primary goal here? is it to update the code with recent changes?

should it preserve local changes that can be preserved?

could we simply abort and mention if there are uncommitted changes? let them figure it out rather than mess with their config if they don’t expect this to happen.

this check introduces a race condition that doesn’t need to exist.

fs.rmSync( dirHashFile, { force: true } );

like most FS calls, performing if checks before doing them (rather than catching errors afterward) opens up a range of bugs and exploits.

https://nodejs.org/api/fs.html#fsrmsyncpath-options

what about build-module, build-style, and build-types, etc…

would it make more sense to read from the .gitignore and apply those rules?

My intention here was to notify the user of any potentially unexpected discrepancy in the gutenberg directory vs. when it was created fresh from the pinned hash.

Essentially, any time there's a change to any source or built file, it could be an indication that there will be unexpected results when building. Looking at this again now with a fresh brain:

• build should likely not be excluded (editing these files directly is incorrect and should be flagged)
• phpunit, tests, etc. should also be excluded.
• docs, platform-docs, etc.

Maintaining this list will be a pain, though.

ultimately we only truly care about the files which get copied into Core, right? would we benefit from reusing that single file mapping from the copy step?

if this is the existing style, then whatever is fine, but these functions without return values are surprising and opaque. they need for the explanatory comments comes from the fact that their behavior is hidden inside them, rather than from being explicit in how we’re handling the responses.

note: checkRepoHash() appears to just do nothing if the directory is entirely missing, but I would have expected from the comment here that some warning would have been raised.

what is the point of sorting the paths?

This was a Claude suggestion.

The sorting ensures the hash is deterministic — the same directory contents always produce the same digest regardless of the order the filesystem returns entries.

I have not validated this yet, but it does make sense to me.

if we didn’t want this warning here we could resolve symlinks with fs.readLinkSync() and then treat them like normal paths.

there are obviously valid reasons not to follow symbolic links, but confusion about whether we are reading the link itself vs. the thing it points to is not one I’m used to seeing.

ah darn, sorry if I leaned too hard into this and we still have to call out to the shell.

there is also spawnSync() if you don’t want all the setup and error code.

function runScript( scriptPath, args = [] ) {
	const response = spawnSync( process.execPath, [ scriptPath, ...args ], {
		cwd: rootDir,
		stdio: 'inherit',
	} );

	if ( 0 === response.status ) {
		return;
	}

	throw response.error;
}